-
Notifications
You must be signed in to change notification settings - Fork 277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: Refactor on-demand-import mechanism #3270
RFC: Refactor on-demand-import mechanism #3270
Conversation
8b1a1aa
to
af41ccb
Compare
555dbd4
to
3d3724f
Compare
Hi -- I know this is draft, but I would prefer we not drop the underscore. I would really like to avoid redefining objects that mirror the external module names. In general, I guess I also didn't realize this was a priority -- did something change that means we have to address this? |
I found that 2 packages are effectively optional dependencies (in the sense that the corresponding import statements are embedded in try/except blocks), but migrating them to use the actual on-demand-import mechanism isn't trivial:
|
3d3724f
to
5d53943
Compare
@matthewturk I don't understand how that's a problem, could you please develop ? A majority of the import statements we have on the main branch already redefine those with the from yt.utilities.on_demand_imports import _h5py as h5py
It's not a priority, but it is something that I can't solve in one go and want to be done with eventually. It's part of my broader effort to solve #3074 |
@matthewturk I could be easily convinced that the leading underscore is a good thing, I'm actually more concerned about consistency. I could rewrite every import statement to use |
My preference, which is I confess somewhat weakly held, is that we should
not be redefining the module names in the modules in which we're importing
them. I'd much prefer defining them with the leading underscore to remove
any ambiguity, and it also helps us to be explicit in our import statements
and to help us catch the appropriate uses.
…On Tue, May 18, 2021 at 1:00 PM Clément Robert ***@***.***> wrote:
@matthewturk <https://github.com/matthewturk> I could be easily convinced
that the leading underscore is a good thing, I'm actually more concerned
about consistency. I could rewrite every import statement to use as.
What's your preference ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3270 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVXO5KZJOU6MP23UPIMR3TOKTKDANCNFSM446XCYDA>
.
|
That sounds fine to me. A concern I do have with the change you're proposing is that it will make this PR even more of a sweeping change, and unless it's merged soon, I will end up maintaining it conflict after conflict for what I can only be assume would be a while. On the other hand, if this goes in as is, there's less of a rush, and I can provide a follow up PR with that change, which will be a breeze to review by itself, so everything can be done in a matter of hours. Does this sound acceptable ? |
Wait, what I'm suggesting is actually to *preserve* the previous behavior
-- import as the real module name, but in the on-demand section, define
them with a leading underscore. It should result in *fewer* files being
touched.
…On Tue, May 18, 2021 at 1:08 PM Clément Robert ***@***.***> wrote:
That sounds fine to me. A concern I do have with the change you're
proposing is that it will make this PR even more of a sweeping change, and
unless it's merged soon, I will end up maintaining it conflict after
conflict for what I can only be assume would be a while. On the other hand,
if this goes in as is, there's less of a rush, and I can provide a follow
up PR with that change, which will be a breeze to review by itself, so
everything can be done in a matter of hours. Does this sound acceptable ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3270 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVXO5KGNG2ZC5QBVEFRY3TOKUKZANCNFSM446XCYDA>
.
|
then I don't understand your argument. :/ |
OK -- I would like to engage on this, but I am trying to find the mental bandwidth to work on the 4.0 stuff. I'm sorry; I have to check out just for a little while, and try to get the last little bit of push. :) Would it be possible to return to this after the 4.0 release, and we can have a high-bandwidth low-latency chat over video or audio? And in the interim, freeze the PR for the time being? |
You got it. |
I only want to comment for now on the issue of importing yt_astro_analysis. I don't like that we do that. We only do it for the halo plot annotation. I think it might make more sense to move that plot callback into yt_astro_analysis as it's specific for halo analysis anyway. In my view, this is line with moving analysis_modules to yt_astro_analysis in the first place. I probably would have done it, but didn't catch it at the time. |
I 100% agree with this, thanks @brittonsmith |
c7404f4
to
d4d9e9a
Compare
d4d9e9a
to
f3ba287
Compare
7c5fab1
to
3eea809
Compare
b6d5b0d
to
ad23206
Compare
This is currently stuck because for some reason, tests that require netCDF4 dataset are failing. I can reproduce this locally by running pytest against this file from yt.utilities.answer_testing.framework import requires_ds
requires_ds("Orbit/orbit_hdf5_chk_0000")(lambda: None) Traceback
note that these two lines work perfectly when run directly from the python runtime.
|
d3caca1
to
ad23206
Compare
close + reopening to retrigger test but I have no particular expectations |
ad23206
to
c51c63c
Compare
c51c63c
to
e226035
Compare
2cd7193
to
605c1b5
Compare
I still can't understand why this is failing NetCDF4 tests. I vaguely feel like the real issue is upstream but in any case this PR is both more controversial than I anticipated at the time and not manageable. Overall it doesn't seem worth the effort of keeping it alive. |
PR Summary
This is another attempt at refactoring the on-demand-import mechanism (supersedes #3246).
The original branch has become virtually impossible to fix, so I'm retrying here with incremental updates.
fix #3237
TODO
_
prefixes in on demand import names (reason: right now the prefix is effectively ignored in import statements via theas
keyword, so there's a precedent of importing stuff from the on demand module as drop-in replacements)NotAModule
class. should be trivial after the previous step is checked (NOT APPLICABLE, see comment bellow)_targets
list a dictionary, and maybe other nitpicks)